-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-99729: Unlink frames before clearing them #100030
Conversation
Not sure if we want buildbots on this (normally I would run them, but I also don't want to further delay the releases with a bunch of extra CI checks). I'm confident in the fix, though. |
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 6210626 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Thanks a lot for fixing this ❤️ I am executing the buildbots at least to see the quick ones to double check there is nothing unexpected. I will manually test Refleaks after before the release. |
Hm, looks like we need to skip the new test on webassembly builds. |
🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit af155c7 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Looks good. I think the buildbot failures are unrelated. |
Thanks @brandtbucher for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Sorry, @brandtbucher and @pablogsal, I could not cleanly backport this to |
Thanks for fixing this @brandtbucher! ❤️
Please, ping me as soon as you have the manual backport ready |
I opened it yesterday: #100047 |
Thanks for your patience with this.
I decided to switch strategies mid-fix after a discussion with @markshannon this morning: instead of hacking the frame to look "incomplete" just before teardown, I think unlinking the frame before clearing it (instead of after) is a better approach that is easier to reason about and keeps extra code out of the hot path when popping frames.
This is going to need a manual backport to 3.11. I'll start on it now to hopefully unblock releases.
CC: @pablogsal